feat(stats): give the collector an outcome ledger - #68
Conversation
PR SummaryLow Risk Overview
Reviewed by Cursor Bugbot for commit ec61136. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
A well-tested, behaviour-neutral addition of an Outcome vocabulary and a per-operation outcome ledger to stats.Collector. No blockers; a misplaced doc comment and two invalid-value handling choices are worth addressing before the tracker starts populating these counts.
Findings: 0 blocking | 4 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion]
InclusionTracker.recordOutcomestill passes bare string literals ("dropped_at_cap","expired") as the metric label, while this PR introducesOutcome.String()producing those same strings. Two parallel vocabularies for one concept will drift; when the tracker is wired up in a follow-up, switch those call sites to theOutcomeconstants so the "one-way door" strings have a single source. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
| // recordOperation counts one attempt for key and, on success, adds its latency | ||
| // to that operation's samples. The bound is the same one recordLatency applies | ||
| // to the pooled window. | ||
| // RecordOutcome counts one terminal outcome under the key the send path already |
There was a problem hiding this comment.
[suggestion] RecordOutcome was inserted between recordOperation's doc comment and recordOperation itself, with no blank line between the two comment blocks. Go treats the whole run of lines 91-102 as RecordOutcome's doc comment, so godoc now shows "recordOperation counts one attempt for key..." as the first sentence of RecordOutcome, and recordOperation is left undocumented. Move those three lines back down to directly above func (c *Collector) recordOperation.
|
|
||
| const ( | ||
| // OutcomeCommitted is a receipt reporting a successful status. | ||
| OutcomeCommitted Outcome = iota |
There was a problem hiding this comment.
[suggestion] OutcomeCommitted is the zero value, so any Outcome field or variable that is never explicitly set records as a successful commit. Given that Committed-vs-Failed is the distinction this type exists to make, a zero-value slip inflates goodput silently and in the safest-looking direction. Consider reserving zero for an unset/invalid state (an unexported outcomeUnset Outcome = iota before OutcomeCommitted) so a missed assignment is detectable rather than counted as success.
| // It takes the same lock as RecordTransaction, and callers reach it from more | ||
| // than one goroutine: a block match and a reap sweep both report outcomes. | ||
| func (c *Collector) RecordOutcome(key OperationKey, outcome Outcome) { | ||
| if outcome >= outcomeCount { |
There was a problem hiding this comment.
[suggestion] An out-of-range outcome is dropped silently. The doc on Outcome states the states partition every accepted transaction and that the conservation identity holds over them at shutdown — a silent drop breaks that identity with no signal, and it is exactly the case a caller bug would produce. Folding it into OutcomeUnknown (or counting it separately) keeps the total conserved and makes the misuse visible in the report.
The collector counts what the sender submitted. The inclusion tracker counts what became of those submissions. Neither holds both halves, so neither can say what fraction of offered work took effect. This adds the vocabulary and the ledger, and changes no behaviour. The tracker does not call it yet. Result names six terminal states. Two distinctions carry the point. Committed and Failed separate a transaction that did what the workload asked from one that burned its gas doing nothing, which an inclusion count cannot tell apart. Unknown separates "the run did not see" from "the chain did not take it", which decides whether a low goodput ratio is a finding about the chain or about the run. Failed names what a receipt reports rather than a cause. A receipt carries one status bit, and separating a revert from an out-of-gas needs a trace call per transaction that the per-block read budget forbids. RecordResult keys on the OperationKey the send path already labels its metrics with, and adds rather than overwrites. Callers reach it from more than one goroutine, because a block match and a reap sweep both report results. The result strings are a one-way door: a dashboard query and a saved report both carry them, so a test pins them. The strings are unchanged by the type's name. Every guard was checked by breaking what it covers. Overwriting instead of adding fails two tests. Folding Failed into Committed fails three. A drifted name fails the string test. Dropping the lock reports a data race. Requirements: TOT-001, TOT-004, TOT-005, TOT-006, TOT-016. Tasks T003 to T006. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cb1480a to
acc718d
Compare
bdchatham
changed the title
The collector counts what the sender submitted. The inclusion tracker counts what became of those submissions. Neither holds both halves, so neither can say what fraction of offered work took effect. This adds the vocabulary and the ledger. Nothing reports an outcome yet. Outcome names six terminal states. Committed and Failed separate a transaction that did what the workload asked from one that burned its gas doing nothing. StatusUnavailable separates "the run did not see" from "the chain did not take it", which decides whether a low goodput ratio is a finding about the chain or about the run. The zero value is a sentinel, not a state. Committed at index 0 would mean an unassigned variable, a switch matching no case, or an early return counts as a commit, silently, which is the failure the type exists to remove. An unset or out-of-range value counts as Unrecorded instead: it has no legitimate producer, so a non-zero count means sei-load has a bug and nothing else explains it. The first one logs, once per run, because a systematic bug would otherwise write a line per transaction. A run never fails over a counting bug. recordOutcome now takes an Outcome rather than a string. The metric label was already fed by bare literals while Outcome.String() produced the same values, so one wire contract had two independent sources and the test pinned the one nothing used. A literal still compiles, so this makes re-splitting unnatural rather than impossible. status_unavailable rather than unknown: a reader seeing unknown beside expired cannot tell a chain finding from a measurement finding, which is the confusion the state exists to prevent. dropped_at_handoff stays, because both alternatives collided with the dispatcher's own load shed, which RunSummary.Dropped already counts and which means the transaction never reached the chain at all. stats/doc.go carries the type map, the three sentinel rules, and the three lock domains. Lifecycle and ownership are marked absent rather than invented, because both describe the tracker loop this change does not add. Guards proven by breaking what they cover: Committed back at index 0, the out-of-range value vanishing, a drifted frozen string. Requirements: TOT-001, TOT-004, TOT-005, TOT-006, TOT-016. Tasks T003 to T006. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bdchatham
changed the title
xreview round 2 — RESOLVED, 0 open findingsFour blinded lenses reviewed The finding that matteredThe zero value of the type was A switch matching no case, a map lookup that misses, an early return before assignment — each silently recorded a commit. TOT-004 forbids exactly that, and the spec lists fail-closed among its own anchors. Now Two lenses reached this independently, and idiom established that the general Go answer runs the other way ( One contract had two sources, and the test pinned the wrong one
Two reviewers reversed themselvesThe dissenter withdrew twice. It expected Platform withdrew a rename it had asked for, after finding both replacements collided with Also fixed
Plus: the merged doc comment ( Guards proven by breaking what they cover
Found outside this diff, needs its own change
|
|
The orphaned-metrics finding from the review is filed as PLT-1081 (Low). It carries the file paths and reference counts, so it survives a re-read: Not a blocker here — different repo — but it is the evidence that this PR's one-way-door comment is describing something that has already happened once. |
1 participant
Closes PLT-1075. First code of PLT-466; PLT-1074 settled the one assumption that gated it.
Why
stats.Collectorcounts what the sender submitted.stats.InclusionTrackercounts what became of those submissions.grep -c Collector stats/inclusion_tracker.goreturns 0 — the tracker holds no reference to the collector, and the collector never learns an outcome. Neither holds both halves, so neither can say what fraction of offered work took effect.A run can report a million accepted, near-perfect inclusion and a healthy p99 while every transaction failed. Nothing in the output says so.
This adds the vocabulary and the ledger. It changes no behaviour — the tracker does not call it yet, the counts stay zero, and a run reads exactly as it does today.
What
Resultnames six terminal states. Two distinctions carry the point of the type:Failednames what a receipt reports, not a cause. A receipt carries one status bit — an explicit revert, an out-of-gas and an invalid opcode all arrive there, and separating them needs a trace call per transaction that the per-block read budget forbids. Calling it a revert would tell an operator the contract rejected the call, which the run cannot see.RecordResult(key, result)keys on theOperationKeyPLT-1025 already landed, and adds rather than overwrites. Callers reach it from more than one goroutine: a block match and a reap sweep both report outcomes.The result strings are a one-way door. A dashboard query and a saved report both carry them, so a rename orphans every one. A test pins them.
Verification
Tests were written before the code. The first red was the compile error:
Then each guard was checked by breaking what it covers, because a test nobody has watched fail has not shown it tests anything:
results[result] = 1instead of++FailedreadsResultCommitted's slot"dropped_at_cap"renamedRecordResultWARNING: DATA RACEThe concurrency test mirrors the tracker's real shape rather than being decoration — two goroutines reporting outcomes is what the head loop and the reap loop will do.
make verifystops atcheck-bindingslocally on a missingsolc. This diff touches no Solidity and CI runs that step properly, but I am not claiming a pass I did not observe.Requirements
TOT-001, TOT-004, TOT-005, TOT-006, TOT-016. Tasks T003 to T006.
Reviewing
Small on purpose. PLT-466 is split into six phases and seven PRs so no single review runs to the size of the contract registry change (+3203 / 35 files, 15 review submissions). This is the foundational one: a type, a method, six fields.
The thing worth arguing about is the state set — six is a claim that these partition every accepted transaction, and the conservation identity in a later PR has to hold over exactly them. Easier to change now than after the tracker populates them.
Renamed
OutcometoResultafter review feedback, and force-pushed rather than stacking a rename commit. Identifiers only: the six strings (committed,failed,expired,dropped_at_cap,dropped_at_handoff,unknown) are unchanged, andTestResultNamesAreStableproves it.stats/outcome.gois nowstats/result.go.The pre-existing
InclusionTracker.recordOutcomeis deliberately untouched — it is a different method emitting OTel counters, and renaming it belongs in the PR that replaces it.